Implementation of the canopy model #288

davidorme · 2024-09-18T13:17:44Z

Description

[Updated to move from WIP to ready to merge].

This PR is to review, adapt and merge in @AmyOctoCat's code on the canopy object from #231.

The main areas of change from that PR are:

The t_model_functions and canopy_functions modules have now settled down to which functions belong in which
module.
Thecanopy_functions module has now been extended to include the canopy vertical structure functions from WIP - 230 create a draft canopy class #231, including the relative canopy radius, stem crown area and stem leaf area at given heights, along with a solver function to find the height at which a canopy layer closes. This includes:
- Rethinking the workflow for functions to avoid repetitive calculation.
- Removing Community object as an argument within the canopy_functions functions and replacing it with explicit arguments for the community properties for each function. This makes the core functions more flexible.
- Adding a bunch of validation for input shapes of canopy function arguments, but making it optional for a speed-up within solvers and classes where the shapes can be assumed correct.
The Canopy object has been reworked to use this new functionality to get to the same end point as in WIP - 230 create a draft canopy class #231.
I've also added unit tests for the canopy functions and a really simple test of the Canopy object.

In addition, from the earlier WIP version of the PR, this code:

is updated to drop use of pandas data structure in favour of simple use of numpy arrays throughout (see Replace use of pandas in pyrealm.demography #292).
updates the t_model_functions to include validation of the input argument shapes and then builds out a deeper unit test suite to a range of input combinations using predictions from the original R implementation.

Things to do later:

Update the docstrings with better docs on the equations and then update the canopy.md document to use this code.
More functionality needed in the Canopy object to use it to generate absorbed radiation per stem per layer and hence generate productivity estimates.

Fixes #286 (issue)

Type of change

New feature (non-breaking change which adds functionality)
Optimization (back-end change that speeds up the code)
Bug fix (non-breaking change which fixes an issue)

Key checklist

Make sure you've run the pre-commit checks: $ pre-commit run -a
All tests pass: $ poetry run pytest

Further checks

Code is commented, particularly in hard-to-understand areas
Tests added that prove fix is effective or that feature works

…py model implementation

codecov-commenter · 2024-09-19T13:44:57Z

Codecov Report

Attention: Patch coverage is 99.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.54%. Comparing base (1f315ba) to head (c2814e3).
Report is 97 commits behind head on develop.

Files with missing lines	Patch %	Lines
pyrealm/demography/canopy.py	97.87%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #288      +/-   ##
===========================================
+ Coverage    95.29%   95.54%   +0.25%     
===========================================
  Files           28       34       +6     
  Lines         1720     2243     +523     
===========================================
+ Hits          1639     2143     +504     
- Misses          81      100      +19

Flag	Coverage Δ
	`95.54% <99.33%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pyrealm/demography/canopy.py

pyrealm/demography/canopy_functions.py

tests/unit/demography/test_t_model_functions.py

omarjamil

Overall, this is a well-structured and thoroughly documented piece of scientific code. The use of numpy, type hinting, and input validation contributes to its robustness. My suggestions are for further improvement of the code, but not critical.

pyrealm/demography/canopy.py

pyrealm/demography/canopy_functions.py

omarjamil · 2024-09-20T11:56:32Z

pyrealm/demography/canopy_functions.py

+    raise ValueError("Invalid shape for the z value.")
+
+
+def calculate_relative_canopy_radius_at_z(


If the inputs array can be reshaped to be broadcastable, then using numpy functions instead of python built-ins e.g. np.power instead of **can help with vectorization and improve performance.

I need to think about (and document) what shapes are sensible here. I believe that M**n is faster if n is a scalar but np.power(M, n) is faster when n is an array? Could get either in using this function.

I think np.power will be faster if you are applying the operation to an array even if the exponent is a scaler. And you can further performance gains if you can apply np.vectorize to functions. Though I feel like I need to back up this with some testing!

omarjamil · 2024-09-20T12:01:44Z

I should add that I am happy to defer to James and Marion in terms of what they consider to important changes to make as they have better code and project knowledge than me.

… 288_292_merge_prep_work

…tation

…-canopy-model

j-emberton

Haven't had a in depth look at the tests yet, but generally this looks OK.
It'd be good to get some thoughts on the potential type inconsistencies for m,n and q_m in canopy_functions.py.
I would prefer to see canopy.py refactored to move the evaluations out of the constructor.

pyrealm/demography/canopy.py

pyrealm/demography/canopy_functions.py

Approving review of early WIP version - needs approval of current proposed final version

davidorme · 2024-09-24T23:08:55Z

@j-emberton The calculate_canopy_q_m and calculate_canopy_z_max_proportion should definitely return float | NDArray but for the other functions, isn't the typing simply saying that this function requires an array of values for q_m, m, n and z_max_proportion. It doesn't matter what type the original function generates as long as any intermediate code packs up values into arrays for use here.

I guess the danger is that users might (not unreasonably) stick the float outputs of those two functions straight into the other functions, but that is intentionally an error - I don't want these functions to support float inputs.

j-emberton · 2024-09-25T06:09:11Z

@j-emberton The calculate_canopy_q_m and calculate_canopy_z_max_proportion should definitely return float | NDArray but for the other functions, isn't the typing simply saying that this function requires an array of values for q_m, m, n and z_max_proportion. It doesn't matter what type the original function generates as long as any intermediate code packs up values into arrays for use here.

I guess the danger is that users might (not unreasonably) stick the float outputs of those two functions straight into the other functions, but that is intentionally an error - I don't want these functions to support float inputs.

Yeah, I see. I think I was being a bit overzealous here, and got into a mindset where those type inconsistencies in calculate_canopy_q_m would be able to propagate into the other functions. But I see that's not the case.

…tter exception messages; partial update tests

davidorme · 2024-09-25T13:35:31Z

@j-emberton I think with that update to the Canopy.__init__ and the improved validation, this is now in good shape. Could you approve? Unless, of course, you don't 😥 ...

j-emberton · 2024-09-25T13:39:36Z

@j-emberton I think with that update to the Canopy.__init__ and the improved validation, this is now in good shape. Could you approve? Unless, of course, you don't 😥 ...

👼 🤔 😈

will go ahead and approve

j-emberton

Thanks for the changes

davidorme added 9 commits September 16, 2024 16:16

Checkout of initial files from #231

5d7fde7

Fixing new import locations in canopy modules

83f0daf

Refining functions and separating canopy functions from specific Cano…

a40501e

…py model implementation

Relocating functions into canopy_functions and renaming imports

3dc0f81

Docstrings and initial unit tests for canopy functions

a6e4e46

Merge branch 'develop' into 286-implementation-of-the-canopy-model

068dddf

More renaming of height to stem_height, fixing tests and docstrings

959d91e

Add crown gap fraction to PlantFunctionalType classes

3eff100

Testing of projected leaf area

1e62bd4

davidorme linked an issue Sep 18, 2024 that may be closed by this pull request

Implementation of the Canopy model #286

Closed

davidorme added 2 commits September 19, 2024 12:53

Added more testing of input shapes, fixing new signature usages

5e294fd

Fixed missing new f_g trait in test_flora inputs

d4f2426

davidorme requested review from MarionBWeinzierl and j-emberton September 19, 2024 13:56

MarionBWeinzierl requested a review from omarjamil September 19, 2024 15:30

davidorme added this to the Demography and allocation model milestone Sep 20, 2024

MarionBWeinzierl reviewed Sep 20, 2024

View reviewed changes

pyrealm/demography/canopy.py Outdated Show resolved Hide resolved

pyrealm/demography/canopy_functions.py Outdated Show resolved Hide resolved

pyrealm/demography/canopy_functions.py Show resolved Hide resolved

tests/unit/demography/test_t_model_functions.py Outdated Show resolved Hide resolved

omarjamil previously approved these changes Sep 20, 2024

View reviewed changes

This was referenced Sep 20, 2024

Add the light allocation process to the Canopy #289

Closed

Add helper functions for PFT geometry and canopy shape. #290

Closed

Replace use of pandas in pyrealm.demography #292

Merged

davidorme added 7 commits September 22, 2024 10:46

Merge branch '291-look-at-replacing-pandas-in-pyrealmdemography' into…

2890eb4

… 288_292_merge_prep_work

Clearing out old Series.to_numpy() casting

95759ad

Fixing broken reference to stem_height in docstring

dcda1a0

Array based testing of T model functions using inputs from R implemen…

894c2bb

…tation

Extending T Model unit test suite to all T model functions

dc7940b

Adding optional input validation to T model functions

4b8a533

Updating regression tests

527c9cb

davidorme added 5 commits September 23, 2024 13:01

Fixing tmodel unit tests for new failure mode

49c8b35

Merge branch 'develop' into 286-implementation-of-the-canopy-model

e1eb3d7

Merge branch '288_292_merge_prep_work' into 286-implementation-of-the…

88c6103

…-canopy-model

Fixed docstring and default from @omarjamil

901b790

Fixes from @MarionBWeinzierl

6a13444

davidorme marked this pull request as ready for review September 23, 2024 15:03

davidorme requested review from omarjamil and MarionBWeinzierl September 23, 2024 15:03

j-emberton reviewed Sep 24, 2024

View reviewed changes

Refactor of Canopy.__init__ to isolate init from calculation

9a5d1d2

davidorme added 3 commits September 25, 2024 09:40

Updated typing on q_m and p_zm functions

1656156

Merge canopy validation functions to avoid duplication and provide be…

1b4b812

…tter exception messages; partial update tests

Updating the rest of the test_canopy_functions

c2814e3

j-emberton self-requested a review September 25, 2024 13:39

j-emberton approved these changes Sep 25, 2024

View reviewed changes

davidorme merged commit 2e98cb6 into develop Sep 25, 2024
12 checks passed

davidorme deleted the 286-implementation-of-the-canopy-model branch September 25, 2024 13:55

davidorme mentioned this pull request Sep 27, 2024

Calculate the canopy structure across a community. #133

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of the canopy model #288

Implementation of the canopy model #288

davidorme commented Sep 18, 2024 •

edited

Loading

codecov-commenter commented Sep 19, 2024 •

edited

Loading

omarjamil left a comment

omarjamil Sep 20, 2024

davidorme Sep 20, 2024

omarjamil Sep 20, 2024

omarjamil commented Sep 20, 2024

j-emberton left a comment

davidorme commented Sep 24, 2024

j-emberton commented Sep 25, 2024

davidorme commented Sep 25, 2024

j-emberton commented Sep 25, 2024

j-emberton left a comment

		raise ValueError("Invalid shape for the z value.")


		def calculate_relative_canopy_radius_at_z(

Implementation of the canopy model #288

Implementation of the canopy model #288

Conversation

davidorme commented Sep 18, 2024 • edited Loading

Description

Type of change

Key checklist

Further checks

codecov-commenter commented Sep 19, 2024 • edited Loading

Codecov Report

omarjamil left a comment

Choose a reason for hiding this comment

omarjamil Sep 20, 2024

Choose a reason for hiding this comment

davidorme Sep 20, 2024

Choose a reason for hiding this comment

omarjamil Sep 20, 2024

Choose a reason for hiding this comment

omarjamil commented Sep 20, 2024

j-emberton left a comment

Choose a reason for hiding this comment

davidorme commented Sep 24, 2024

j-emberton commented Sep 25, 2024

davidorme commented Sep 25, 2024

j-emberton commented Sep 25, 2024

j-emberton left a comment

Choose a reason for hiding this comment

davidorme commented Sep 18, 2024 •

edited

Loading

codecov-commenter commented Sep 19, 2024 •

edited

Loading